feat(autoware_perception_msgs): implement the proposed universal radar messages #120
Conversation
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
|
The one think I am over the fence on is about whether we should normalize the probabilities for [0.0, 1.0] or keep them [0.0, 100.0]. How do you guys think? |
I am very surprised someone actually used 0->100 to represent probabilities. At least, any new implementation should have them normalized 0->1. |
There was a problem hiding this comment.
It's probably worth it to write have a small support library that
- ensures that RadarInfo is received before any RadarObjects are handled
- provides accessors for RadarObject fields that auto-convert/validate using the received RadarInfo
It seems quite easy to screw up as a user of these messages otherwise - e.g. orientation, is it deg, rad, something else?
| uint8 MEASUREMENT_STATUS_PREDICTED = 1 | ||
| uint8 MEASUREMENT_STATUS_NEW = 2 | ||
| uint8 MEASUREMENT_STATUS_UNKNOWN = 3 | ||
| uint8 MEASUREMENT_STATUS_INVALID = 255 |
There was a problem hiding this comment.
Not sure if these values are chosen for backward compatibility, but e.g. Protobuf recommends to make 0 the invalid option such that any zero-initialized message is invalid by default.
There was a problem hiding this comment.
Nice!
This has changed a bit from the proposal, with the generalization of the field info being the biggest difference I can see. While this is clean, I wonder if it is good to explicitly define the info fields in the radar info message.
Another comment - whether to not this ends up being the catalyst to merge sensing and perception messages, these are quite complex message definitions so adding an explanation for the new types in the README would be very much appreciated (once the review of the contents themselves is complete).
While we don't use the radar detections currently, would it be worth defining it to make the info fields easier to understand/use in driver implementation?
| @@ -0,0 +1,8 @@ | |||
| std_msgs/String field_name | |||
| bool min_value_available | |||
There was a problem hiding this comment.
For the availability bools, I remember there was a discussion about using NaN (I guess the above suggestion by @xmfcx , using -1.0f may work for resolution, but it's a bit dangerous as requires the user to check an apparently irrelevant field for availability). I'm guessing having the bools as proposed is cleaner?
There was a problem hiding this comment.
Another way of doing this would be to use a field like
float32[<=1] min_value
which is functionally equivalent to an std::optional<float32>.
This would remove the need for the boolean field, and the user is forced to check if the value is present. Probably a bit ugly to use since it shows up as a dynamic bounded array, not an optional 🥲
Co-authored-by: Taekjin LEE <technolojin@gmail.com>
Co-authored-by: Taekjin LEE <technolojin@gmail.com>
Co-authored-by: Mete Fatih Cırıt <xmfcx@users.noreply.github.com>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…on msgs counterpart Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
xmfcx
changed the title
| @@ -6,7 +6,8 @@ ament_auto_find_build_dependencies() | |||
|
|
|||
| set(msg_files | |||
| "msg/GnssInsOrientation.msg" | |||
| "msg/GnssInsOrientationStamped.msg") | |||
| "msg/GnssInsOrientationStamped.msg" | |||
There was a problem hiding this comment.
Could you put the parenthesis back up? To keep consistent.
Description
This PR implemented the universal radar messages discussed in
https://github.com/orgs/autowarefoundation/discussions/5264
How was this PR tested?
Used together with tier4/nebula#284
to check the field contents
Notes for reviewers
None.
Effects on system behavior
None.